[office] Add regression tests for PDFDocumentPage. #101

dcaliste · 2016-11-25T20:43:05Z

This is quite WIP, but worth to share I think. Using the qmltestrunner and the methodology that was described in the document @pvuorela sent me, I've added some regression tests to the PDF renderer page.

Of course, it's not testing extensively all possibilities, but it already ensures that:

broken PDF displays the placeholder.
protected PDF can only be opened with the right password.
number of pages and current page are correct.
goto link and url link can be tapped and context menu or scroll are activated.
search functionality is basically working (no result and matching navigation).

It's not testing yet the selection capability and the annotation ones.

It must be run with the device display on but the device can be either in portrait or landscape orientation. I tried to make the position checks resolution independant but I guess it will require some adjustments for other device than JollaC I tested it on.

Finally, I needed to add some accessors in the QML parts and I added a correction in pdfrenderthread.cpp to avoid a race condition when changing documents without changing PDFDocumentPage QML object. Oh, I forgot, I modified also the scrollTo() and contentAt() functions for the latter not to constraint the calculated point into the view, and delegate this constraint to the scroll routine instead.

Is it going in the right direction ? Should tests be written in a different way ? What do you think ?

dcaliste · 2016-12-02T13:00:16Z

Has been rebased on master after scrolling changes for search match.

pvuorela

Some of the pixels and positions in the tests scare me, how easily those will get broken or not work on different device. Can have but just hope there won't be too much maintenance burden on them.

Some minor comments but can merge even if tests don't cover everything etc, just as long the running code side is clean end tests provide some use.

pvuorela · 2016-12-02T13:09:34Z

pdf/pdfrenderthread.cpp

    default:
+        if (d->document->isLocked())
+            return;


Default case leaks memory? Should warn? Could also note code changes in commit message and why they were done.

Well, this is dirty fallback solution for the following issue:

load a valid PDF file and render it;

load a locked PDF file with the same PdfDocument instance (by calling setDocument());

having a race condition in PdfCanvas that may call updatePaintNode() from the rendering thread before the page count is reset to zero by line 758.
In that situation you may have some rendering job in the queue before the document is unlocked, which segfault later in pdfjob.cpp#61 when calling page() on a non-existing one.

What do you think ? Is it better to try to cure this race condition, that doesn't happen in practice because we're not reusing the same PdfDocument instance each time ; or should I add a comment here mentioning the reason ?

Besides, writing this, I notice that I should convert the Q_ASSERT() I've added in pdfjob.cpp#61 as a return statement for the same race condition reason with a document smaller than the previous one.

For things that cannot happen in normal operation of the application I wouldn't mind if there are small workarounds avoiding wrong behavior in testing. However, those should be clearly marked with comments, maybe even doing qWarning(), so it's possible to catch the problem if application behavior ever changes.

Ok, I've added a comment before the test. I've also added some qWarning() in pdfjob.cpp to signal something unusual happening (and avoid segfaults also).

pvuorela · 2016-12-02T13:11:30Z

rpm/sailfish-office.spec

+%package tests
+Summary: Unitary tests for %{name}
+License: GPLv2
+Group: System/Base


Think license gets inherited. Not sure if System/Base is better, could just skip that as well.

Ok, removed.

pvuorela · 2016-12-02T13:29:02Z

plugin/PDFDocumentPage.qml

+    property alias document: pdfDocument
+    property alias placeHolder: placeHolderLoader
+    property alias toolbar: toolbar
+


Commonly not fond of adding very much code used only for testing, but can have some. Just thinking it could be marked for such usage so it's not accidentally removed as cleanup. Maybe could also have all testability properties of the file grouped together?

I try to tune the tests so added accessors may have a sense from a public API point of view, like exposing the toolbar item so any external user of the PDFDocumentPage item may control better the toolbar behaviour for their own needs.

Well, for the moment, I've simply added a comment on every added accessors…

dcaliste · 2016-12-05T11:38:57Z

Some of the pixels and positions in the tests scare me, how easily those will get broken or not work on different device. Can have but just hope there won't be too much maintenance burden on them.

I have the same concern, as I said in the introduction. I've tried to use reduced page coordinates as often as possible. I may have left mistakes also… The actual coordinates used for mouse inputs should currently all be in reduce coordinates to ensure that it doesn't depend on screen resolution… The tricky parts are when checking that the screen has scrolled of the appropriated amount to ensure that a reduce coordinate point is visible or in a corner…

Maybe readability (and maintainance burden) will be improved by writing check functions specific to our case (i.e. check actual flickable position from reduced coordinates and constraints), something like:

function ensureFlickableAt(reducedCoord, screenXPosition (in [0;1]), screenYPosition (in [0;1])) {
            tryCompare(_mainPage.documentItem, "scrolling", false)
            var pt = _mainPage.documentItem.contentAt(…)
            fuzzyCompare(_mainPage.documentItem.contentX, Math.round(…, xErrorMargin)
            fuzzyCompare(_mainPage.documentItem.contentY, Math.round(…, yErrorMargin)
}

I'll try to polish the tests in that direction during the week.

dcaliste · 2016-12-08T19:30:35Z

I've factorised some of the code repeated here and there in the test to press at given page coordinates. I've also checked that all coordinates are given in reduced page coordinates. I think the tests should be valid on other screen resolutions also.

pvuorela · 2016-12-09T13:54:06Z

Tried running on jolla 1, which worked fine, and tablet, which didn't fully:

********* Start testing of qmltestrunner *********
Config: Using QtTest library 5.6.2, Qt 5.6.2 (i386-little_endian-ilp32-qreal_float shared (dynamic) release build; by GCC 4.8.3 20140401 (Mer 4.8.3-1))
PASS   : qmltestrunner::search::initTestCase()
PASS   : qmltestrunner::search::test_match()
FAIL!  : qmltestrunner::search::test_match_back_navigate() Compared values are not the same with delta(10)
   Actual   (): 3555
   Expected (): 4920
   Loc: [/opt/tests/sailfish-office/tst_PDFDocumentPage.qml(238)]
FAIL!  : qmltestrunner::search::test_match_reopen() Compared values are not the same
   Actual   (): easy
   Expected (): 
   Loc: [/opt/tests/sailfish-office/tst_PDFDocumentPage.qml(315)]
FAIL!  : qmltestrunner::search::test_match_reopen_finalize() property searchIconized
   Actual   (): false
   Expected (): true
   Loc: [/opt/tests/sailfish-office/tst_PDFDocumentPage.qml(321)]
PASS   : qmltestrunner::search::test_no_match()
FAIL!  : qmltestrunner::search::test_no_match_finalize() property searchIconized
   Actual   (): false
   Expected (): true
   Loc: [/opt/tests/sailfish-office/tst_PDFDocumentPage.qml(279)]
PASS   : qmltestrunner::search::cleanupTestCase()
PASS   : qmltestrunner::gotoLink::initTestCase()
PASS   : qmltestrunner::gotoLink::test_00_setup()
FAIL!  : qmltestrunner::gotoLink::test_goto() Compared values are not the same with delta(10)
   Actual   (): 502
   Expected (): 491
   Loc: [/opt/tests/sailfish-office/tst_PDFDocumentPage.qml(210)]
PASS   : qmltestrunner::gotoLink::test_goto_back()
PASS   : qmltestrunner::gotoLink::cleanupTestCase()
PASS   : qmltestrunner::urlLink::initTestCase()
PASS   : qmltestrunner::urlLink::test_00_setup()
FAIL!  : qmltestrunner::urlLink::test_contextMenu() Compared values are not the same with delta(30)
   Actual   (): 236.11767578125
   Expected (): 200.18218994140625
   Loc: [/opt/tests/sailfish-office/tst_PDFDocumentPage.qml(118)]
PASS   : qmltestrunner::urlLink::test_contextMenu_finalize()
FAIL!  : qmltestrunner::urlLink::test_deviceRotation() Compared values are not the same with delta(30)
   Actual   (): 236.11767578125
   Expected (): 200.18218994140625
   Loc: [/opt/tests/sailfish-office/tst_PDFDocumentPage.qml(118)]
PASS   : qmltestrunner::urlLink::test_deviceRotation_finalize()
PASS   : qmltestrunner::urlLink::cleanupTestCase()
PASS   : qmltestrunner::basics::initTestCase()
PASS   : qmltestrunner::basics::test_currentPage()
PASS   : qmltestrunner::basics::test_pages()
PASS   : qmltestrunner::basics::cleanupTestCase()
PASS   : qmltestrunner::protectedPDF::initTestCase()
QDEBUG : qmltestrunner::protectedPDF::test_placeholderBroken() [D] Poppler::Debug::qDebugDebugFunction:44 - "Error: Incorrect password"
PASS   : qmltestrunner::protectedPDF::test_placeholderBroken()
PASS   : qmltestrunner::protectedPDF::cleanupTestCase()
PASS   : qmltestrunner::brokenPDF::initTestCase()
QDEBUG : qmltestrunner::brokenPDF::test_placeholderBroken() [D] Poppler::Debug::qDebugDebugFunction:44 - "Error: May not be a PDF file (continuing anyway)"
QDEBUG : qmltestrunner::brokenPDF::test_placeholderBroken() [D] Poppler::Debug::qDebugDebugFunction:44 - "Error: Couldn't find trailer dictionary"
QDEBUG : qmltestrunner::brokenPDF::test_placeholderBroken() [D] Poppler::Debug::qDebugDebugFunction:44 - "Error: Couldn't find trailer dictionary"
QDEBUG : qmltestrunner::brokenPDF::test_placeholderBroken() [D] Poppler::Debug::qDebugDebugFunction:44 - "Error: Couldn't read xref table"
PASS   : qmltestrunner::brokenPDF::test_placeholderBroken()
PASS   : qmltestrunner::brokenPDF::cleanupTestCase()
Totals: 23 passed, 7 failed, 0 skipped, 0 blacklisted
********* Finished testing of qmltestrunner *********

Think some emulated clicks don't find their targets. Option would be to trigger click signal manually, but that would require exposing more toolbar internals.

dcaliste · 2016-12-09T14:20:09Z

Think some emulated clicks don't find their targets.

For the search cases, I agree, it is supposed to push on back, next and clear buttons, but actions are not done. I'll check the tap positions in the test for this.
"FAIL! : qmltestrunner::gotoLink::test_goto()", this may be due to wrong fuzzy comparison (goto links are moving the page with a padding of Theme.paddingLarge, while the comparison is done with Theme.paddingSmall… I'll check this and if the target coordinates are correct.
for the context menu opening position, I'll double check the opening reduced coordinates and the size of the fuzzy for the comparisons.

Thank you for testing this on different screen resolutions.

dcaliste · 2019-02-19T11:58:58Z

Rebased on master, works on JollaC, will test on Tablet later (@chriadam).

dcaliste force-pushed the tests branch from 32560a3 to dd7769f Compare December 2, 2016 12:59

pvuorela reviewed Dec 2, 2016

View reviewed changes

dcaliste force-pushed the tests branch from dd7769f to e8b9b68 Compare December 5, 2016 11:30

dcaliste force-pushed the tests branch 2 times, most recently from d8aa799 to 537cbac Compare December 8, 2016 12:44

dcaliste force-pushed the tests branch from 537cbac to 8fa000f Compare December 9, 2016 13:41

dcaliste force-pushed the tests branch 2 times, most recently from ac78109 to e82819a Compare December 16, 2016 14:20

[office] Add regression tests for PDFDocumentPage.

0f41cbe

dcaliste force-pushed the tests branch from e82819a to 0f41cbe Compare February 19, 2019 11:57

ploum

46fc21a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[office] Add regression tests for PDFDocumentPage. #101

[office] Add regression tests for PDFDocumentPage. #101

dcaliste commented Nov 25, 2016

dcaliste commented Dec 2, 2016

pvuorela left a comment

pvuorela Dec 2, 2016

dcaliste Dec 5, 2016

pvuorela Dec 5, 2016

dcaliste Dec 5, 2016

pvuorela Dec 2, 2016

dcaliste Dec 5, 2016

pvuorela Dec 2, 2016

dcaliste Dec 5, 2016

dcaliste commented Dec 5, 2016 •

edited

Loading

dcaliste commented Dec 8, 2016

pvuorela commented Dec 9, 2016

dcaliste commented Dec 9, 2016

dcaliste commented Feb 19, 2019

[office] Add regression tests for PDFDocumentPage. #101

Are you sure you want to change the base?

[office] Add regression tests for PDFDocumentPage. #101

Conversation

dcaliste commented Nov 25, 2016

dcaliste commented Dec 2, 2016

pvuorela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcaliste commented Dec 5, 2016 • edited Loading

dcaliste commented Dec 8, 2016

pvuorela commented Dec 9, 2016

dcaliste commented Dec 9, 2016

dcaliste commented Feb 19, 2019

dcaliste commented Dec 5, 2016 •

edited

Loading